Skip to content

Significant Events Endpoint for Article as a Living Document experiment#2

Open
tonisevener wants to merge 50 commits intomasterfrom
significant-changes
Open

Significant Events Endpoint for Article as a Living Document experiment#2
tonisevener wants to merge 50 commits intomasterfrom
significant-changes

Conversation

@tonisevener
Copy link
Owner

Copy link
Collaborator

@joewalsh joewalsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great overall! Just found one minor issue

var titleDict = domainDict[keyTitle];
if (titleDict) {
titleDict[item.revid] = item;
titleDict.maxedOut = calculateCacheForTitleIsMaxedOut(titleDict);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This titleDict is a copy of the value inside domainDict so updating it doesn't update the value inside of domainDict. This could be fixed by moving domainDict[keyTitle] = titleDict; outside of the if/else block so that the updated value is set in the domainDict in either case. I'm not sure if there's a way to get reference semantics here so you could update directly

titleDict[item.revid] = item;
titleDict.maxedOut = calculateCacheForTitleIsMaxedOut(titleDict);
domainDict[keyTitle] = titleDict;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above but for the domainDict, should be fixed by moving significantChangesCache[req.params.domain] = domainDict; below out of the if/else block


return Object.assign({
uncachedRevisions: uncachedRevisions,
cachedOutput: cachedOutput
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed cachedOutput is always empty, even on subsequent requests for the same title. Seems to be fixed by making the changes suggested below

domainDict = {};
domainDict[keyTitle] = titleDict;
significantChangesCache[req.params.domain] = domainDict;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the comments above, and that || in JS works kind of like ?? in Swift, this could be updated to:

function setSignificantChangesCache(req, title, item) {
    const keyTitle = keyTitleForCache(req, title);
    var domainDict = significantChangesCache[req.params.domain] || {};
    var titleDict = domainDict[keyTitle] || {};
    titleDict[item.revid] = item;
    titleDict.maxedOut = calculateCacheForTitleIsMaxedOut(titleDict);
    domainDict[keyTitle] = titleDict;
    significantChangesCache[req.params.domain] = domainDict;
}

}

// clean out from talk page cache
// todo: why on earth do we get a talkPageTitle is not a function error here?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you had:

    var talkPageTitle = talkPageTitle(req);

the function is already shaded by the variable name. Could be fixed by renaming the function to getTalkPageTitle or using a different variable name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants